Skip to content

[AMDGPU] Added hot-block-rematerialize pass #126331

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open

[AMDGPU] Added hot-block-rematerialize pass #126331

wants to merge 25 commits into from

Conversation

@adam-yang
Copy link
Contributor

@adam-yang adam-yang commented Feb 8, 2025 •
edited
Loading

Note: Before this is ready for review, I intend to remove AMDGPUMirSyncDependency and AMDGPUMirDivergenceAnalysis. They were basically forks of the IR versions of the passes from before the upstream MIR versions were available.

I have the code path that uses MachineUniformityInfo and the divergence related unit test passes with it, but I'd like to do more testing before deleting the other ones.

@github-actions
Copy link

github-actions bot commented Feb 8, 2025 •
edited
Loading

✅ With the latest revision this PR passed the undef deprecator.

@github-actions
Copy link

github-actions bot commented Feb 8, 2025 •
edited
Loading

✅ With the latest revision this PR passed the C/C++ code formatter.

@dstutt dstutt requested review from jayfoad and perlfu February 26, 2025 17:30
@dstutt
Copy link
Collaborator

dstutt commented Feb 26, 2025

Added Jay and Carl fyi (still needs cleanup)

@dstutt dstutt self-requested a review February 26, 2025 17:31
@perlfu
Copy link
Contributor

perlfu commented Feb 27, 2025

Since this is only a draft and MirDivergence parts are going away I only I had a quick look.
I wonder if parts like AMDGPULatencyTracker should be full analysis passes, rather than just utilities?

jayfoad
jayfoad reviewed Feb 27, 2025
View reviewed changes
llvm/lib/Target/AMDGPU/AMDGPUOccupancyAndLatencyHelper.cpp Outdated
Copy link
Contributor

@jayfoad jayfoad Feb 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to #include <cmath> for this. But perhaps it would be better to use an integer-only power operation (not sure if we already have one anywhere in LLVM).

@jayfoad
Copy link
Contributor

jayfoad commented Feb 27, 2025 •
edited
Loading

Is this intended to be run just before pre-RA MachineScheduler?

Update: I see the pass currently requires SSA form, so it would have to be run earlier than that. Where would you put it in the pass pipeline?

@dstutt
Copy link
Collaborator

dstutt commented Feb 27, 2025

We've got some work in the pipeline to allow pre-RA machinescheduler to run in SSA form as well, so that might be ok.
Also, this might work well with some other work we're considering to add spilling as a separate pass before RA (but in SSA form).

adam-yang and others added 19 commits March 17, 2025 20:24
@adam-yang
Added rematerialize pass and test.
b6eb3b3
@adam-yang
Fixed build, and added simple tests that exercise major code paths
7739842
@adam-yang
Test renames, only keeping the required flags for the tests
3539ab3
@adam-yang
Using the mir uniformity analysis instead, which DOES require SSA; bu…
a33f944 
…t I don't see any reason why we can't just require SSA
@adam-yang
In block remat AND making v to s slightly more robust
2215b79
@adam-yang
clang-format
d36a4ae
@adam-yang
Added option to enable it in the target profile
bf396df
@adam-yang
Fix PHI node handling in regpressure tracker
c64c4e4
@adam-yang
Fixed the PHI issue
3dc22d4
@adam-yang
Removed old forks of things
29eca4a
@adam-yang
Clang format and warnings.
6b011fb
@adam-yang
First batch of formatting changes
eb4f8c1
@adam-yang
Batch 2
78ab7f3
@adam-yang
More cleanup
d8b6711
@adam-yang
More cleanups
f8eb7fb
@adam-yang
Possibly the last batch of cleanup
0600e2f
@adam-yang
Additional cleanup + format
84d8dd8
@adam-yang
Added cmath
303a401
@adam-yang
Wrong place for std header
971e556
@adam-yang adam-yang force-pushed the amdgpu_hot_block_remat branch from ede9e76 to 971e556 Compare March 18, 2025 03:24
@adam-yang adam-yang marked this pull request as ready for review March 18, 2025 05:21
@jayfoad
Copy link
Contributor

jayfoad commented Mar 25, 2025

Hi @adam-yang, I've done some testing with this patch and noticed that options -amdgpu-remat-enable-hot-block-remat-aggressive and -amdgpu-remat-enable-sub-exp-remat seem to cause many crashes in the compiler. Are you interested in test cases for these? Or are these options not expected to be working yet?

@adam-yang
Copy link
Contributor Author

adam-yang commented Mar 25, 2025

Hi @adam-yang, I've done some testing with this patch and noticed that options -amdgpu-remat-enable-hot-block-remat-aggressive and -amdgpu-remat-enable-sub-exp-remat seem to cause many crashes in the compiler. Are you interested in test cases for these? Or are these options not expected to be working yet?

Yes could you send them to me please.

@jayfoad
Copy link
Contributor

jayfoad commented Mar 26, 2025

OK, here's a somewhat reduced test case for a crash with -amdgpu-remat-enable-sub-exp-remat. The verifier reports "Bad machine code: Found PHI instruction after non-PHI".
r1.txt

@jayfoad
Copy link
Contributor

jayfoad commented Mar 26, 2025

Here's one for -amdgpu-remat-enable-hot-block-remat-aggressive which fails an assertion "Invalid RC for virtual register": r2.txt

@adam-yang
Copy link
Contributor Author

adam-yang commented Mar 27, 2025

Here's one for -amdgpu-remat-enable-hot-block-remat-aggressive which fails an assertion "Invalid RC for virtual register": r2.txt

This revealed some incorrect assumptions in this PR. Lanes are assumed to be 32-bit, but new 16-bit subregs have been added so lanes are now 16-bit. I'll have to go through the change to fix some more issues related to this.

@adam-yang adam-yang mentioned this pull request Apr 21, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jayfoad jayfoad jayfoad left review comments

@perlfu perlfu Awaiting requested review from perlfu

@dstutt dstutt Awaiting requested review from dstutt

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@adam-yang @dstutt @perlfu @jayfoad